Skip to content

Reimplement playback backed with unit tests#277

Merged
jaredjj3 merged 38 commits intomasterfrom
playback
Apr 5, 2025
Merged

Reimplement playback backed with unit tests#277
jaredjj3 merged 38 commits intomasterfrom
playback

Conversation

@jaredjj3
Copy link
Collaborator

@jaredjj3 jaredjj3 commented Mar 17, 2025

This PR fixes #276. It will be included in ^0.1.5

Demos:

Screen.Recording.2025-04-05.at.6.37.30.PM.mov
Screen.Recording.2025-04-05.at.6.36.29.PM.mov
demo.mov

I split Sequence into two independently tested classes called Timeline and CursorFrame. Timeline lists moments of interest while CursorFrame are specific interpolation instructions for a Cursor. I also fixed a bug where MeasureSequenceIterator would not skip endings correctly when there were more than 2.

@jaredjj3 jaredjj3 self-assigned this Mar 17, 2025
@jaredjj3
Copy link
Collaborator Author

@rvilarl, I finished making the test suite. Would you take a look at suggest anything I'm missing please?
https://github.com/stringsync/vexml/blob/ab9e4f622e053d5a2431f5640a434be698b7a104/tests/unit/playback/sequencefactory.test.ts

Note

I decided against using parameterized it.each tests because I know the assertions are going to be complex and I want this to be easier to debug.

@jaredjj3
Copy link
Collaborator Author

jaredjj3 commented Mar 27, 2025

I’m finally going to have some time this weekend to surge on this. Just gathering some thoughts.

I think playback.Sequence’s complexity is a sign that it’s doing too much. It assumes its consumers (i.e. playback.Cursor for now) will interpolate values forcing it to figure out that information up front.

Instead, I want to generate a simpler data structure up front: playback.Timeline. It will contain a playback.TimelineEvent[] and have methods that make it easy to query by index or timestamp.

Some types that come to mind:

type ElementTransition = {
  type: 'start' | 'stop';
  element: PlaybackElement;
}

type TimelineEvent = {
  type: 'element-transition';
  time: Duration;
  transitions: ElementTransition[];
} | {
  // For repeats, codas, etc. This is used to inform interpolation to the measure end.
  type: 'jump';
  originVoiceEntryKey: VoiceEntryKey;
  destinationVoiceEnteyKey: VoiceEntryKey;
} | {
  // Also used to inform measure-end interpolation
  type: 'system-end';
  systemKey: SystemKey;
};

Then, it’s the cursor’s responsibility to create a list of interpolation instructions from the timeline. The cursor will expose its own types in its eventing. I think the timeline should remain private to vexml and users should rely on playback.Cursor for timeline queries.

Ultimately, I think this approach will make playback.Timeline and playback.Cursor much easier to develop and test because I think it’s scoped better.

One of the risks is that I might be underestimating how difficult it would be for playback.Cursor to create interpolation instructions from a playback.Timeline — this is where the current complexity exists in 0.1.4. I’m also not fully settled how I want to communicate special transitions (such as retriggering repeated notes vs. tied notes).

@jaredjj3
Copy link
Collaborator Author

I finished the Timeline implementation. When implementing the tests, I found a number of edge cases I didn't consider before.

I ended up stringifying the events for assertions so it's easier for a human to discern if the event sequence is correct or not. It does add complexity, especially if the TimelineEvent type changes, but I think it's worth it. Otherwise, it was really difficult to read the tests — when assertions failed, it took a while for me to orient what really is failing.

The remaining work is to create separate cursor types and logic to take a Timeline and dispatch events that more accurately describe changes as discussed in #276.

@jaredjj3
Copy link
Collaborator Author

jaredjj3 commented Apr 1, 2025

I'm currently experimenting with the following idea of Cursor creating a CursorFrame[]:

interface CursorFrame {
    readonly tRange: DurationRange;
    readonly xRange: util.NumberRange;
    readonly yRange: util.NumberRange;
    readonly activeElements: PlaybackElement[];
    getHints(previousFrame: CursorFrame): CursorFrameHint[];
};

export type CursorFrameHint = RetriggerHint | SustainHint;

export type RetriggerHint = {
  type: 'retrigger';
  untriggerElement: PlaybackElement;
  retriggerElement: PlaybackElement;
};

export type SustainHint = {
  type: 'sustain';
  previousElement: PlaybackElement;
  currentElement: PlaybackElement;
};

@rvilarl, for your use case you'll need to check the CursorFrame.getHints to see if any elements should get special treatment. Otherwise, you can just use CursorFrame.activeElements to see what notes should be active. Let me know if you see any problems with this.

@jaredjj3
Copy link
Collaborator Author

jaredjj3 commented Apr 4, 2025

I pretty much finished the new implementation and it's now backed by tests. When manually testing in the dev server, I notice that some documents with repeat endings have "dead zones". I suspect that there is some discontinuity in the timeline.

Even though I do have coverage for repeats and repeats with endings, I'll add more test cases around this.

@jaredjj3
Copy link
Collaborator Author

jaredjj3 commented Apr 4, 2025

A shortlist of deadzone documents:

  • 45a-SimpleRepeat.musicxml
  • repeats-alternate-endings-simple.musicxml
  • repeats-alternate-endings-advanced.musicxml

@jaredjj3
Copy link
Collaborator Author

jaredjj3 commented Apr 4, 2025

In repeats-alternate-endings-simple.musicxml, there's something weird going on with the frames:

image
t: [0ms - 2400ms]
x: [left(element(0)) - left(element(1))]
y: [top(system(0), part(0)) - bottom(system(0), part(0))]

t: [2400ms - 4800ms]
x: [left(element(1)) - right(measure(0))] <- ⚠️ This is wrong
y: [top(system(0), part(0)) - bottom(system(0), part(0))]

t: [4800ms - 7200ms]
x: [left(element(0)) - right(measure(1))]
y: [top(system(0), part(0)) - bottom(system(0), part(0))]

t: [7200ms - 9600ms]
x: [left(element(2)) - right(measure(0))]
y: [top(system(0), part(0)) - bottom(system(0), part(0))]

t: [9600ms - 12000ms]
x: [left(element(0)) - left(element(1))]
y: [top(system(0), part(0)) - bottom(system(0), part(0))]

t: [12000ms - 14400ms]
x: [left(element(1)) - right(measure(0))]
y: [top(system(0), part(0)) - bottom(system(0), part(0))]

t: [14400ms - 16800ms]
x: [left(element(0)) - right(measure(1))]
y: [top(system(0), part(0)) - bottom(system(0), part(0))]

t: [16800ms - 19200ms]
x: [left(element(3)) - right(measure(0))]
y: [top(system(0), part(0)) - bottom(system(0), part(0))]

@jaredjj3 jaredjj3 marked this pull request as ready for review April 5, 2025 22:45
@jaredjj3 jaredjj3 merged commit 9963568 into master Apr 5, 2025
1 check passed
@jaredjj3 jaredjj3 deleted the playback branch April 5, 2025 22:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Cursor active notes

1 participant